Refactor collection traits and return types by sethrj · Pull Request #2351 · celeritas-project/celeritas

sethrj · 2026-04-03T16:23:30Z

A number of places that could have been using __ldg are not, due to implicit casting to Span (see #2348) and inconsistent behavior between host and device. Now, instead of using LdgSpan for device and Span for host, this always returns LdgSpan when returning from const collections for consistency: syntax/conversion errors will behave the same for both cases. Due to the overly relaxed implicit span constructor, a number of LdgSpan are now being returned but converted to Span under the hood.

This PR also removes some of the extra templating and redundant traits classes used in collection. It should slightly reduce the amount of object code due to simplified templates, will make debugging easier due to one nested class implementation, and generally should be easier to read.

github-actions · 2026-04-03T16:37:33Z

Test summary

5 980 files 9 697 suites 19m 48s ⏱️
2 257 tests 2 215 ✅ 42 💤 0 ❌
33 710 runs 33 583 ✅ 127 💤 0 ❌

Results for commit 5031ed8.

♻️ This comment has been updated with latest results.

codecov · 2026-04-04T11:28:02Z

Codecov Report

❌ Patch coverage is 96.72131% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.26%. Comparing base (a58f23c) to head (5031ed8).
⚠️ Report is 2 commits behind head on develop.

Files with missing lines	Patch %	Lines
src/celeritas/em/msc/UrbanMsc.hh	0.00%	0 Missing and 1 partial ⚠️
src/corecel/data/detail/CollectionImpl.hh	87.50%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2351      +/-   ##
===========================================
+ Coverage    87.25%   87.26%   +0.01%     
===========================================
  Files         1377     1377              
  Lines        43850    43838      -12     
  Branches     13874    13848      -26     
===========================================
- Hits         38263    38257       -6     
+ Misses        4527     4522       -5     
+ Partials      1060     1059       -1

Files with missing lines	Coverage Δ
src/celeritas/em/data/UrbanMscData.hh	`100.00% <ø> (ø)`
src/celeritas/em/msc/detail/UrbanMscHelper.hh	`100.00% <100.00%> (ø)`
src/celeritas/em/params/UrbanMscParams.cc	`94.59% <ø> (ø)`
src/celeritas/global/CoreTrackView.hh	`87.50% <100.00%> (ø)`
.../optical/surface/model/GridReflectivityExecutor.hh	`100.00% <100.00%> (ø)`
src/celeritas/track/SimData.hh	`95.83% <ø> (ø)`
src/corecel/data/Collection.hh	`100.00% <100.00%> (ø)`
src/corecel/data/DeviceAllocation.hh	`76.47% <ø> (ø)`
src/corecel/data/DeviceVector.hh	`13.15% <ø> (ø)`
src/corecel/grid/NonuniformGrid.hh	`100.00% <100.00%> (ø)`
... and 6 more

... and 8 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Add explicit .get() calls when accessing values returned by Collection subscript operators that now return LdgWrapper<T const> instead of T const&. This fixes compilation errors in boolean contexts, comparisons, and arithmetic operations where implicit conversion is not available. Changes include: - Boolean context checks in assertions (VolumeSurfaceView.hh) - Conditional expressions with wrapped IDs (GridReflectivityExecutor.hh) - Comparison operations in grid searches (NonuniformGrid.hh) - Arithmetic operations with wrapped offsets (VolumePathAccumulator.hh) - Test assertions comparing expected values with wrapped data Prompt: "I've made a code change so that `LdgWrapper<T const>` rather than `T const&` is being returned in a number of places. Build the entire code base, fixing implicit conversion/comparison by accessing the returned/dereferenced value with `.get()` explicitly." Assisted-by: GitHub Copilot

This reverts commit 38edd6e.

Copilot

Pull request overview

This PR refactors Collection trait/storage plumbing and adjusts call sites/tests to consistently return LDG-aware wrappers/spans from const collections, improving host/device consistency and preventing accidental loss of LDG acceleration.

Changes:

Simplify Collection internals by collapsing redundant trait/storage helpers and centralizing storage on a single StorageT.
Introduce AutoLdgSpan and switch const collection accessors/call sites to use .get() where LDG wrappers are now returned.
Update multiple unit tests and a few geometry/physics call sites to accommodate LDG wrapper return types.

Reviewed changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
test/corecel/math/Algorithms.test.cu	Adjusts kernel math calls for LDG wrapper element access (`.get()` / casts).
test/corecel/data/Collection.test.cc	Updates expectations for LDG wrapper/span return types and validates `data()` pointer types.
test/celeritas/optical/Cherenkov.test.cc	Updates test expectations to extract values from LDG wrappers with `.get()`.
test/celeritas/em/WentzelVIMsc.test.cc	Updates test expectations to extract values from LDG wrappers with `.get()`.
src/orange/OrangeTrackView.hh	Updates volume instance ID access for LDG wrapper semantics and clarifies lambda return type.
src/geocel/VolumeSurfaceView.hh	Ensures validity checks use `.get()` for LDG wrapper results.
src/geocel/VolumePathAccumulator.hh	Uses `.get()` to extract numeric offsets from LDG wrappers before arithmetic.
src/corecel/math/Algorithms.hh	Refactors `fastpow` SFINAE to return-type form.
src/corecel/grid/NonuniformGrid.hh	Updates comparisons/search predicates to extract values from LDG wrappers via `.get()`.
src/corecel/data/DeviceVector.hh	Minor doc cleanup (`\todo`) and removes an unused include.
src/corecel/data/DeviceAllocation.hh	Removes unused include and trims outdated TODO commentary.
src/corecel/data/detail/FillInvalid.hh	Removes an unused include.
src/corecel/data/detail/CollectionImpl.hh	Major refactor of collection traits/storage/copy helpers and LDG span selection.
src/corecel/data/Collection.hh	Updates `Collection` API/type aliases, storage member, inlined accessors, and moves free `make_ref` helpers.
src/corecel/cont/LdgSpan.hh	Adds `AutoLdgSpan` and tweaks `remove_ldg_wrapper` signature to match wrapper-based span.
src/celeritas/track/SimData.hh	Removes unused include after refactor ripple.
src/celeritas/optical/surface/model/GridReflectivityExecutor.hh	Uses `.get()` for ID truthiness checks with LDG wrappers.
src/celeritas/global/CoreTrackView.hh	Simplifies dereference of `ObserverPtr` returned by `data()` (drops redundant `.get()`).
src/celeritas/em/params/UrbanMscParams.cc	Removes unused type alias after ID/type refactor.
src/celeritas/em/msc/UrbanMsc.hh	Updates ID access to use `.get()`; adjusts member constness/comment for kernel argument passing.
src/celeritas/em/msc/detail/UrbanMscHelper.hh	Extracts IDs/indices appropriately under LDG wrapper semantics and updates indexing logic.
src/celeritas/em/data/UrbanMscData.hh	Moves `UrbanParMatId` type alias to a shared location and updates dependent field types.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-06T20:54:16Z

src/corecel/data/detail/CollectionImpl.hh

+    // Const cast is OK because the only time it's used is when this is called
+    // with Ownership::reference and the caller is doing T* -> const T*
+    auto* data = const_cast<T*>(src.data());
+    auto size = src.size();


copy_collection unconditionally const_casts the source pointer (auto* data = const_cast<T*>(src.data());). This removes compile-time const-safety and makes it possible to create an Ownership::reference (mutable Span) view into a const Ownership::value collection (e.g., from a Collection<..., Ownership::value> const&), which would allow mutating elements of a const container and is undefined behavior. Consider overloading copy_collection for mutable vs const sources (or only casting when the source is known to be mutable) so that assigning/building Ownership::reference from a const-owned collection fails to compile rather than silently casting away const.

esseivaju

Looks good, two minor comments

esseivaju · 2026-04-06T20:48:52Z

src/corecel/math/Algorithms.hh

-template<class T, std::enable_if_t<std::is_floating_point<T>::value, bool> = true>
-inline CELER_FUNCTION T fastpow(T a, T b)
+template<class T>
+inline CELER_FUNCTION std::enable_if_t<std::is_floating_point<T>::value, T>


Suggested change

inline CELER_FUNCTION std::enable_if_t<std::is_floating_point<T>::value, T>

inline CELER_FUNCTION std::enable_if_t<std::is_floating_point_v<T>, T>

esseivaju · 2026-04-06T21:07:59Z

src/corecel/data/detail/CollectionImpl.hh

-    using SpanT = Span<type>;
-    using SpanConstT = Span<const_type>;
+    using const_type = T;  //!< Return type is *mutable* for reference!
+    using SpanConstT = Span<T>;


-- Commented on the wrong line, should be line 70 -- Should we also redefine StorageT since SpanT is redefined? We wouldn't expect a user to have modifiable storage through Ownership::const_reference

Suggested change

using SpanConstT = Span<T>;

using SpanConstT = Span<T>;

using StorageT = SpanT;

sethrj added 6 commits April 3, 2026 08:59

Rename impl

8798df4

Simplify collection traits

2d01834

Fix obsever pointer

bd1501c

Fix collection doc rendering

189c115

Rename impl_ -> s_

c01775d

Simplify storage validation

7727245

sethrj requested review from amandalund and pcanal April 3, 2026 16:23

sethrj added core Software engineering infrastructure (corecel) minor Refactoring or minor internal changes/fixes labels Apr 3, 2026

sethrj added 2 commits April 4, 2026 06:51

Fix build error

bcbd053

Merge remote-tracking branch 'upstream/develop' into collection-refactor

e44d931

sethrj added 7 commits April 4, 2026 08:19

Add test/clean up impl

93e4533

Try to fix coverage

3e029d0

Merge remote-tracking branch 'upstream/develop' into collection-refactor

0a59cff

Native auto ldg span

640bec5

Make UrbanParMatId a global type

23bda20

Always return ldg wrapper, even when not on device

e73a6c4

sethrj requested a review from elliottbiondo as a code owner April 6, 2026 14:43

fixup! Always return ldg wrapper, even when not on device

e8ab743

sethrj requested review from esseivaju and removed request for elliottbiondo April 6, 2026 16:18

Fix cuda build

4bcbaf1

sethrj changed the title ~~Simplify collection backend implementation~~ Use cached loading in device code Apr 6, 2026

Fix collection documentation

5031ed8

This reverts commit 38edd6e.

sethrj changed the title ~~Use cached loading in device code~~ Refactor collection traits and return types Apr 6, 2026

sethrj requested a review from Copilot April 6, 2026 20:44

Copilot started reviewing on behalf of sethrj April 6, 2026 20:45 View session

Copilot AI reviewed Apr 6, 2026

View reviewed changes

esseivaju approved these changes Apr 6, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor collection traits and return types#2351

Refactor collection traits and return types#2351
sethrj wants to merge 18 commits intoceleritas-project:developfrom
sethrj:collection-refactor

sethrj commented Apr 3, 2026 •

edited

Loading

Uh oh!

github-actions bot commented Apr 3, 2026 •

edited

Loading

Uh oh!

codecov bot commented Apr 4, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 6, 2026

Uh oh!

esseivaju left a comment

Uh oh!

esseivaju Apr 6, 2026

Uh oh!

esseivaju Apr 6, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	inline CELER_FUNCTION std::enable_if_t<std::is_floating_point<T>::value, T>
	inline CELER_FUNCTION std::enable_if_t<std::is_floating_point_v<T>, T>

	using SpanConstT = Span<T>;
	using SpanConstT = Span<T>;
	using StorageT = SpanT;

Conversation

sethrj commented Apr 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Apr 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test summary

Uh oh!

codecov bot commented Apr 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

esseivaju left a comment

Choose a reason for hiding this comment

Uh oh!

esseivaju Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

esseivaju Apr 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sethrj commented Apr 3, 2026 •

edited

Loading

github-actions bot commented Apr 3, 2026 •

edited

Loading

codecov bot commented Apr 4, 2026 •

edited

Loading

esseivaju Apr 6, 2026 •

edited

Loading